Change Setup Wizard redirect to work on all relevant pages #7581

renatho · 2024-04-05T14:15:27Z

Proposed Changes

There was a change on WP 6.5, where it doesn't reload the page when installing a plugin from the plugins directory through "Plugins > Add New". If the plugin is uploaded from a zip file the behavior continues the same as previously.

The Sensei Setup Wizard used to work immediately after we activate the plugin. With the recent change, our logic doesn't work anymore.

After a discussion (p1712321247992169-slack-C02NWDZBL0H), we decided to keep the intended behavior from WordPress core, and the Setup Wizard will only open after the user navigate in the WP admin.

Considered alternative

I thought of using this JS event https://github.com/WordPress/WordPress/blob/9bb63fd66503d139e0b3e8498c2a5a80cb3507a1/wp-admin/js/updates.js#L1123 and force a redirect when it's the Sensei activation, but it doesn't align with what WP core is proposing.

Testing Instructions

Deactivate Sensei LMS plugin.
Remove the option sensei_installed from the database.
Navigate to WP Admin > Plugins > Add New Plugin.
Search for "Sensei LMS".
Click on "Activate" (notice the new behavior for WP 6.5, where it's an ajax request and won't redirect to the setup wizard).
Click on "Users" in the menu.
Check that you are not redirected to the Setup Wizard.
Click on Sensei LMS, and check that you are redirected to the Setup Wizard (the same should happen when clicking on Sensei pages, Dashboard page and Plugins page).
Repeat the test in WP 6.4, and make sure it continues working properly when clicking on "Activate", as it used to work.

Pre-Merge Checklist

Context

See p1712321247992169-slack-C02NWDZBL0H

…splayed

github-actions · 2024-04-05T14:21:11Z

Test the previous changes of this PR with WordPress Playground.

renatho · 2024-04-05T14:23:02Z

phpcs.xml.dist

@@ -81,4 +81,13 @@
 		<exclude-pattern>**/views/*</exclude-pattern>
 		<exclude-pattern>tests/bootstrap.php</exclude-pattern>
 	</rule>
+
+	<rule ref="WordPress.WP.Capabilities">


It adds the Sensei custom capability, so PHPCS issues don't happen when it's used.

renatho · 2024-04-05T14:24:07Z

includes/admin/class-sensei-setup-wizard.php

@@ -446,7 +448,7 @@ public function get_woocommerce_connect_data() {
 	 *
 	 * @return stdClass Extension with status.
 	 */
-	private function get_feature_with_status( $extension, $installing_plugins, $selected_plugins ) {
+	private function get_feature_with_status( $extension, $installing_plugins, $selected_plugins ) { // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed -- Called by a public deprecated method.


Just a PHPCS fix.

renatho · 2024-04-05T14:24:10Z

includes/admin/class-sensei-setup-wizard.php

@@ -545,7 +547,7 @@ public static function close_wccom_install() {

 		if (
 			isset( $_SERVER['HTTP_REFERER'] ) &&
-			0 === strpos( $_SERVER['HTTP_REFERER'], 'https://woocommerce.com/checkout' ) && // phpcs:ignore sanitization ok.
+			0 === strpos( $_SERVER['HTTP_REFERER'], 'https://woocommerce.com/checkout' ) && // phpcs:ignore -- sanitization ok.


Just a PHPCS fix.

renatho · 2024-04-05T14:24:16Z

includes/admin/class-sensei-setup-wizard.php

@@ -496,7 +498,7 @@ public function get_sensei_extensions( $clear_active_plugins_cache = false ) {
 		}

 		$extensions = array_map(
-			function( $extension ) use ( $installing_plugins, $selected_plugins ) {
+			function ( $extension ) use ( $installing_plugins, $selected_plugins ) {


Just a PHPCS fix.

renatho · 2024-04-05T14:25:23Z

includes/admin/class-sensei-setup-wizard.php

@@ -93,7 +93,7 @@ public function __construct() {
 			add_action( 'current_screen', [ $this, 'remove_notices_from_setup_wizard' ] );
 			add_action( 'admin_notices', [ $this, 'setup_wizard_notice' ] );
 			add_action( 'admin_init', [ $this, 'skip_setup_wizard' ] );
-			add_action( 'admin_init', [ $this, 'activation_redirect' ] );
+			add_action( 'current_screen', [ $this, 'activation_redirect' ] );


It's changed because now we want to use the get_current_screen, and it's not ready in the admin_init hook.

renatho · 2024-04-05T14:29:04Z

includes/admin/class-sensei-setup-wizard.php

@@ -221,6 +220,8 @@ protected function redirect_to_setup_wizard() {
 	 * Render app container for setup wizard.
 	 */
 	public function render_wizard_page() {
+		// Delete option when the Setup Wizard is loaded, so it doesn't redirect anymore.
+		delete_option( 'sensei_activation_redirect' );


Moved from the redirect method to here because we want it removed anytime the Setup Wizard is loaded.

I don't think it would happen in a normal flow, but this makes more sense in case some logic is changed in the future.

renatho · 2024-04-05T14:34:10Z

includes/class-sensei.php

@@ -1166,7 +1166,7 @@ public function activate_sensei() {
 			// Do not enable the wizard for sites that are created with the onboarding flow.
 			if ( 'sensei' !== get_option( 'site_intent' ) ) {

-				set_transient( 'sensei_activation_redirect', 1, 30 );
+				update_option( 'sensei_activation_redirect', 1 );


Changed to option, so if the user delays more than 30 seconds to click around, the Setup Wizard will still open in the relevant pages.

github-actions · 2024-04-05T17:06:30Z

Test the previous changes of this PR with WordPress Playground.

github-actions · 2024-04-05T17:32:14Z

Test the previous changes of this PR with WordPress Playground.

codecov · 2024-04-05T17:37:16Z

Codecov Report

Attention: Patch coverage is 89.18919% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 51.81%. Comparing base (7742f33) to head (564fdde).
Report is 4 commits behind head on trunk.

❗ Current head 564fdde differs from pull request most recent head 693851e. Consider uploading reports for the commit 693851e to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk    #7581      +/-   ##
============================================
+ Coverage     51.76%   51.81%   +0.04%     
- Complexity    11323    11326       +3     
============================================
  Files           641      641              
  Lines         48185    48208      +23     
  Branches        470      470              
============================================
+ Hits          24943    24978      +35     
+ Misses        22861    22849      -12     
  Partials        381      381

Files	Coverage Δ
includes/class-sensei-data-cleaner.php	`87.75% <ø> (ø)`
includes/class-sensei.php	`24.89% <0.00%> (ø)`
includes/admin/class-sensei-setup-wizard.php	`55.00% <91.66%> (+11.34%)`	⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0132d5c...693851e. Read the comment docs.

github-actions · 2024-04-05T18:51:56Z

Test the previous changes of this PR with WordPress Playground.

…edirect

renatho · 2024-04-05T19:09:45Z

includes/admin/class-sensei-setup-wizard.php

+			// Only redirects for admin users.
+			|| ! current_user_can( 'manage_sensei' )
+			// Check if it's an admin screen that should redirect.
+			|| ! $this->should_current_page_redirect_to_wizard()


Now the redirect runs when the user accesses the pages we used to display the Setup Wizard notice (Dashboard, Plugins, and Sensei pages).

renatho · 2024-04-05T19:12:11Z

includes/admin/class-sensei-setup-wizard.php

+	 *
+	 * @return boolean
+	 */
+	private function should_current_page_redirect_to_wizard() {


I separated the checks for the notice and the redirect because the notice is very ugly in Sensei Home and we want the redirect if the user navigates to the Sensei Home.

github-actions · 2024-04-05T19:15:29Z

Test the previous changes of this PR with WordPress Playground.

Imran92

LGTM! Just left some minor suggestions

Imran92 · 2024-04-08T12:03:59Z

tests/unit-tests/admin/test-class-sensei-setup-wizard.php

+	/**
+	 * Testing if redirect option is cleared on setup wizard rendering.
+	 */
+	public function testRenderWizardPageClearsRedirectOption() {


Should we name the test function like we do for our other tests? Like this p6rkRX-35r-p2 (test**_When**_Does**)?

Sure! I didn't do that to follow the old standard of this file, but to make it write I changed the whole file here now: 2d7b8eb

Imran92 · 2024-04-08T12:04:31Z

tests/unit-tests/admin/test-class-sensei-setup-wizard.php

+
+		Sensei()->setup_wizard->render_wizard_page();
+
+		$this->assertFalse( get_option( 'sensei_activation_redirect', false ) );


Maybe we can divide these in Arrange Act Assert sections?

Imran92 · 2024-04-08T12:05:18Z

tests/unit-tests/admin/test-class-sensei-setup-wizard.php

@@ -247,8 +267,6 @@ public function testActivationRedirectNoAdmin() {
 			->method( 'redirect_to_setup_wizard' );

 		$setup_wizard_mock->activation_redirect();
-
-		$this->assertNotFalse( get_transient( 'sensei_activation_redirect' ), 'Transient should not be removed' );


The next test still mentions Transient even though we are no longer using transients, should we rename?

Fixed with the 2d7b8eb

Imran92 · 2024-04-08T12:09:37Z

tests/unit-tests/admin/test-class-sensei-setup-wizard.php


-		set_transient( 'sensei_activation_redirect', 1, 30 );
+		update_option( 'sensei_activation_redirect', 1 );


Instead of using mock here, is it possible to test it using the Sensei_Test_Redirect_Helpers trait? You can see an example here

sensei/tests/unit-tests/admin/home/tasks/task/test-class-sensei-home-task-pro-upsell.php

Lines 54 to 69 in 0132d5c

public function testMarkCompleteAndRedirect_WhenCalled_TriesToRedirectToRightPage() {

/* Arrange. */

$this->login_as_admin();

$this->prevent_wp_redirect();

$redirect_location = '';

/* Act. */

try {

Sensei_Home_Task_Pro_Upsell::mark_completed_and_redirect();

} catch ( Sensei_WP_Redirect_Exception $e ) {

$redirect_location = $e->getMessage();

}

/* Assert. */

$this->assertSame( 'https://senseilms.com/sensei-pro/?utm_source=plugin_sensei&utm_medium=upsell&utm_campaign=sensei-home', $redirect_location );

}

. Maybe it can farther improve the reliability of the test? Just a suggestion, not necessary at all

Sure! Thank you for the suggestion! Updated here: 693851e

Very nice! <3 Now they are covering much more realistically than the mock!

github-actions · 2024-04-08T20:29:27Z

Test the previous changes of this PR with WordPress Playground.

Imran92

LGTM! 🚀

renatho added 4 commits April 5, 2024 11:02

Add changelog

4050769

Add custom capability

8771e3c

Fix PHPCS comment

a62211d

Update logic to open Setup Wizard on the same pages the notice was di…

534655a

…splayed

renatho added this to the 4.23.1 milestone Apr 5, 2024

renatho self-assigned this Apr 5, 2024

renatho changed the title ~~Change/setup wizard redirect~~ Change Setup Wizard redirect to work on all relevant pages Apr 5, 2024

renatho commented Apr 5, 2024

View reviewed changes

Improve code documentation

05c4bb1

Add tests

e645908

Add Sensei Home to the list of pages to open the setup wizard

4b4ddf5

Update check to use one list for the notice and another one for the r…

564fdde

…edirect

renatho commented Apr 5, 2024

View reviewed changes

renatho requested a review from a team April 5, 2024 19:12

renatho marked this pull request as ready for review April 5, 2024 19:12

Imran92 previously approved these changes Apr 8, 2024

View reviewed changes

Update tests format

2d7b8eb

Use redirect helpers to test the setup wizard redirect

693851e

renatho dismissed Imran92’s stale review via 693851e April 8, 2024 20:25

renatho requested a review from Imran92 April 8, 2024 20:28

Imran92 approved these changes Apr 9, 2024

View reviewed changes

renatho merged commit 9410aa2 into trunk Apr 9, 2024
23 checks passed

renatho deleted the change/setup-wizard-redirect branch April 9, 2024 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Setup Wizard redirect to work on all relevant pages #7581

Change Setup Wizard redirect to work on all relevant pages #7581

renatho commented Apr 5, 2024 •

edited

Loading

github-actions bot commented Apr 5, 2024

renatho Apr 5, 2024 •

edited

Loading

renatho Apr 5, 2024

renatho Apr 5, 2024

renatho Apr 5, 2024

renatho Apr 5, 2024

renatho Apr 5, 2024

renatho Apr 5, 2024

github-actions bot commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

codecov bot commented Apr 5, 2024 •

edited

Loading

github-actions bot commented Apr 5, 2024

renatho Apr 5, 2024

renatho Apr 5, 2024

github-actions bot commented Apr 5, 2024

Imran92 left a comment

Imran92 Apr 8, 2024

renatho Apr 8, 2024

Imran92 Apr 8, 2024

renatho Apr 8, 2024

Imran92 Apr 8, 2024

renatho Apr 8, 2024

Imran92 Apr 8, 2024

renatho Apr 8, 2024

Imran92 Apr 9, 2024

github-actions bot commented Apr 8, 2024

Imran92 left a comment


		Sensei()->setup_wizard->render_wizard_page();

		$this->assertFalse( get_option( 'sensei_activation_redirect', false ) );


		set_transient( 'sensei_activation_redirect', 1, 30 );
		update_option( 'sensei_activation_redirect', 1 );

	public function testMarkCompleteAndRedirect_WhenCalled_TriesToRedirectToRightPage() {
	/* Arrange. */
	$this->login_as_admin();
	$this->prevent_wp_redirect();
	$redirect_location = '';

	/* Act. */
	try {
	Sensei_Home_Task_Pro_Upsell::mark_completed_and_redirect();
	} catch ( Sensei_WP_Redirect_Exception $e ) {
	$redirect_location = $e->getMessage();
	}

	/* Assert. */
	$this->assertSame( 'https://senseilms.com/sensei-pro/?utm_source=plugin_sensei&utm_medium=upsell&utm_campaign=sensei-home', $redirect_location );
	}

Change Setup Wizard redirect to work on all relevant pages #7581

Change Setup Wizard redirect to work on all relevant pages #7581

Conversation

renatho commented Apr 5, 2024 • edited Loading

Proposed Changes

Considered alternative

Testing Instructions

Pre-Merge Checklist

Context

github-actions bot commented Apr 5, 2024

renatho Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

codecov bot commented Apr 5, 2024 • edited Loading

Codecov Report

github-actions bot commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2024

Imran92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 8, 2024

Imran92 left a comment

Choose a reason for hiding this comment

renatho commented Apr 5, 2024 •

edited

Loading

renatho Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading